fix(dnd): update useDescription to prevent unnecessary re-renders#9738
fix(dnd): update useDescription to prevent unnecessary re-renders#9738reidbarber wants to merge 6 commits intomainfrom
Conversation
reidbarber
changed the title
| function getOrCreateDynamicDescriptionNode(descriptionKey: string) { | ||
| let desc = dynamicDescriptionNodes.get(descriptionKey); | ||
| if (!desc) { | ||
| let id = `react-aria-description-${descriptionId++}`; |
There was a problem hiding this comment.
in the case that multiple copies are loaded, this could create conflicting ids, better to use id generation like crypto.randomUUID
There was a problem hiding this comment.
or put the id generation into the hooks and make use of useId
| }, [description]); | ||
|
|
||
| useLayoutEffect(() => { | ||
| return () => { |
There was a problem hiding this comment.
if you move this up to be in the first useLayoutEffect, then you can de-duplicate the refCount removal, always handle it in the cleanup of that effect
It'll make it more readable as well because creation and cleanup associated will be right next to each other
reidbarber
changed the title
|
Build successful! 🎉 |
|
Build successful! 🎉 |
## API Changes
@react-aria/utils/@react-aria/utils:useDescription useDescription {
description?: string
+ descriptionKey?: string
returnVal: undefined
} |
| } | ||
| }; | ||
|
|
||
| const DESCRIPTION_KEYS = { |
There was a problem hiding this comment.
If half the app is in ja-JP and the other half is in en-US, these keys will conflict and some english descriptions will end up in jp or vice versa?
Probably have to use the translated string itself as the key
| }, [description, descriptionKey, isDynamic]); | ||
|
|
||
| desc.refCount++; | ||
| useLayoutEffect(() => { |
There was a problem hiding this comment.
I still think this can be moved up into the previous layout effect, negating the need to call cleanup on line 82
| * a stable node is shared by key and its text content updates in place as the | ||
| * description changes. | ||
| */ | ||
| export function useDescription(description?: string, descriptionKey?: string): AriaLabelingProps { |
There was a problem hiding this comment.
In theory we could do this without a special key - basically try to keep the id the same over time. When the description changes, if the ref count is zero, reuse the same id for the new description.
|
hmm actually, what do you think about using |
|
@devongovett that would definitely be better. I'll close this and try that out in a separate PR soon. |
4 participants
Closes #2504
Adds a new opt-in
descriptionKeyoption to the hook that gets used byuseDraganduseVirtualDrop.It solves the following issue with
useDescription:Our new option changes the text content of the existing description node, so the 100 drag buttons do not need to re-render in this case. We need to key these, so that multiple consumers can reliably share the same node.
✅ Pull Request Checklist:
📝 Test Instructions:
In the RAC DnD Table story, open dev tools to inspect the element.
Alternative between clicking and tabbing to switch drag modalities, and observe:
🧢 Your Project: